Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

4084: adds Couchbase as JanusGraph backend #4086

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

4084: adds Couchbase as JanusGraph backend #4086

wants to merge 6 commits into from

Conversation

chedim
Copy link

@chedim chedim commented Oct 31, 2023 •
edited
Loading

Issue #4084
This PR adds couchbase JanusGraph backend and search. The backend is in alfa stage and is not yet recommended for production use.

All the dependencies for the backend are either already in other poms (logback) or use licenses from category a.

Thank you for your time reviewing the PR. Suggestions on the code are highly welcomed as this was our team's first project with JanusGraph.

Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Oct 31, 2023 •
edited
Loading

CLA Not Signed

@chedim
Copy link
Author

chedim commented Oct 31, 2023 •
edited
Loading

CLA Not Signed

* [❌](https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/1543822/77385607/4086/#/?version=2) - login: @chedim / name: Dmitrii Chechetkin . The commit ([d595025](https://github.com/JanusGraph/janusgraph/commit/d595025b681606d8a903613bb5b7e8b7ecac338d)) is not authorized under a signed CLA. [Please click here to be authorized](https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/1543822/77385607/4086/#/?version=2). For further assistance with EasyCLA, [please submit a support request ticket](https://jira.linuxfoundation.org/servicedesk/customer/portal/4).

Working on this internally with Couchbase, should be signed within the week.

porunov
porunov reviewed Nov 2, 2023
View reviewed changes
Copy link
Member

@porunov porunov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @chedim for the contribution!

The documentation build is failing here: https://github.com/JanusGraph/janusgraph/actions/runs/6722438476/job/18291081482?pr=4086

Most likely the new added configurations are not part of the generated configuration options. You could try executing the following to fix that failing check:

mvn clean install -DskipTests=true
git add -A
git commit --amend -s
git push -f

I also see Java 8 build fails here: https://github.com/JanusGraph/janusgraph/actions/runs/6722438464/job/18291060016?pr=4086
However, Java 11 build passes. It could mean that you used some functionality that isn't available in Java 8. You could either rewrite that functionality (Lucene2CouchbaseQLTranslator.java:[160,57]) to be compatible with Java 8 or you could wait until JanusGraph drops support for Java 8. You can see here that there is a plan to drop Java 8 support and make Java 11 minimal required Java version for JanusGraph: #4040

I also left a comment regarding CouchbaseKeyColumnValueStore implementation, but I haven't reviewed the actual code yet.

...uchbase/src/main/java/org/janusgraph/diskstorage/couchbase/CouchbaseKeyColumnValueStore.java Show resolved Hide resolved
@janusgraph-bot janusgraph-bot added the cla: external Externally-managed CLA label Nov 3, 2023
@chedim
4084: Add Couchbase Backend
bbd8f70 
- should fix the documentation issue, also adds documentation for Couchbase storage options
- makes `Lucene2CouchbaseQLTranslator` compatible with Java8
- overrides `KeyColumnValueStore::getMultiSlices` in `CouchbaseKeyColumnValueStore` with a non-blocking implementation

Signed-off-by: Dmitrii Chechetkin <[email protected]>
@chedim
Copy link
Author

chedim commented Nov 13, 2023

The CLA part is taking much longer than I've expected. Escalated it, will post an update when it is signed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@porunov porunov porunov left review comments

@cdegroc cdegroc cdegroc left review comments

Assignees
No one assigned
Labels
cla: external Externally-managed CLA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chedim @cdegroc @porunov @janusgraph-bot